Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

js-api: Add implementation defined limit of 16GiB for 64-bit memories. #100

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Nov 6, 2024

No description provided.

@sbc100 sbc100 requested review from backes and bvisness November 6, 2024 23:07
@sbc100 sbc100 changed the title js-api: An implementation defined limit of 16GiB for 64-bit memories. js-api: Add implementation defined limit of 16GiB for 64-bit memories. Nov 6, 2024
@programmerjake
Copy link

why limit it to 16GiB? pretty reasonable programs can use waay more memory than that...

Copy link
Member

@backes backes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@backes
Copy link
Member

backes commented Nov 7, 2024

why limit it to 16GiB? pretty reasonable programs can use waay more memory than that...

We need some spec'ed limit for Web embeddings to ensure predictability on the Web. We can always decide to raise it later, when the need arises. Any allocation above 16GiB is very unlikely to succeed on many clients anyway, and 16GiB is also the limit that the V8 sandbox currently imposes in Chrome.

@@ -1781,7 +1781,8 @@ In practice, an implementation may run out of resources for valid modules below

<ul>
<li>The maximum size of a table is 10,000,000.</li>
<li>The maximum number of pages of a memory is 65,536.</li>
<li>The maximum size of a 32-bit memory is 65,536 pages (4 GiB).</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually necessary, given that more than 4G isn't even addressable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should just drop this line then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its somewhat useful to have the two limits listed next to each other like this.. even though I guess its redundant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nevermind, the limit itself is pre-existing.

@sbc100 sbc100 merged commit 525a53c into main Nov 7, 2024
8 checks passed
@sbc100 sbc100 deleted the memory_limits branch November 7, 2024 16:21
@bvisness
Copy link
Collaborator

bvisness commented Nov 7, 2024

@sbc100 The implementation limit in Firefox currently stands at 8GiB, not 16. I think we'd be open to raising that limit, but the relevant people are on PTO right now so we'll have to get back to you later. In the meantime, we'd prefer that the spec say 8GiB.

@backes
Copy link
Member

backes commented Nov 7, 2024

Oh, from https://searchfox.org/mozilla-central/source/js/src/wasm/WasmConstants.h#1156 we concluded that Firefox doesn't have any limit currently. Where is the 8GiB limit implemented?

@bvisness
Copy link
Collaborator

bvisness commented Nov 7, 2024

Ah that makes sense, sorry about the confusion. That constant is just for validation, not implementation. Here's the implementation limit:

https://searchfox.org/mozilla-central/source/js/src/wasm/WasmMemory.cpp#253
https://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.h#218

@backes
Copy link
Member

backes commented Nov 8, 2024

Thanks for clarifying, that makes sense!
Wouldn't it be enough to lower the validation limit to 16GiB then? It's always allowed to fail at runtime already before that limit.

We heard from partners that an 8GiB limit wouldn't be hugely helpful though given that a wasm64 program by design always needs more memory than the corresponding wasm32 program. So the 8GiB won't give you substantially more memory than the 4GiB for wasm32.
I would thus propose to work towards raising your implementation limit to 16GiB. We (in V8) are also open to raise the limit further once the need arises.

hubot pushed a commit to v8/v8 that referenced this pull request Nov 8, 2024
This was spec'ed in WebAssembly/memory64#100.

Also assert that the spec'ed size matches the supported size on 64-bit
architectures.

[email protected]

Bug: 41480462
Change-Id: I1b6d08d7cfebc7c6ced71408371e8add2ed9c36c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6003067
Commit-Queue: Clemens Backes <[email protected]>
Reviewed-by: Eva Herencsárová <[email protected]>
Cr-Commit-Position: refs/heads/main@{#97073}
@eqrion
Copy link
Contributor

eqrion commented Nov 8, 2024

We heard from partners that an 8GiB limit wouldn't be hugely helpful though given that a wasm64 program by design always needs more memory than the corresponding wasm32 program. So the 8GiB won't give you substantially more memory than the 4GiB for wasm32. I would thus propose to work towards raising your implementation limit to 16GiB. We (in V8) are also open to raise the limit further once the need arises.

We'll look into it on our side. I don't know if there's anything fundamental about it, or it's just conservative. Some relevant folks are on PTO right now, so it'll take a while. If there is something fundamental about it, we may ask that the implementation limit only grow to 8 instead of 16.

@eqrion
Copy link
Contributor

eqrion commented Nov 15, 2024

We think we'll be able to increase the limit to 16GiB. We're going to do the work here [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1931401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants